-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Advanced partitioning customizations (COMPOSER-2399) #1041
Conversation
Size values in the blueprint can be defined as either integers, representing bytes, or strings with SI unit prefixes for bytes. The conversion for the Filesystem.MinSize happens in both the JSON and TOML unmarshallers. With the new partitioning customizations we will have more conversions of the same type. This commit extracts the handling into a generic function for reusability and convenience. This changes the error messages. Tests have been updated accordingly.
- New structure that's more powerful than the existing array of FilesystemCustomization. - The general structure defines a (minimum) size for the overall disk and an array of partitions. - Each partition has one of three types: - plain: a plain disk partition, meaning partitions with traditional filesystem payloads (xfs, ext4, etc). - lvm: a partition containing an LVM volume group, which in turn will contain one or more LVM logical volumes. - btrfs: a partition containing a btrfs volume, which in turn will contain one or more subvolumes. - The new FilesystemTypedCustomization struct supports a label and fs_type and no minsize, compared to the old FilesystemCustomization. This struct is used to define both the payload of a plain partition and the payload of an LVM logical volume. - The PartitionCustomization has a type and size and embeds three substructures, one for each partition type. Decoding/unmarshalling of the PartitionCustomization is handled by a "typeSniffer" which first reads the value of the "type" field and then deserialises the whole object into a struct that only contains the fields valid for that partition type. This ensures that no fields are set for the substructure of a different type than the one defined in the "type" fields.
The unmarshaller decodes "type" and "minsize" manually and reuses the decode* functions from the JSON unmarshaller for each subtype based on the value of "type".
Add validation functions for DiskCustomization. Three types of validation are added: - Validate(): checks for functionally invalid configurations (bad mountpoint names or invalid structures, etc). The docstring lists all properties that are validated. - ValidateLayoutConstraints(): checks for limitations we imposed ourselves as a matter of policy and support on the structure of the partition table. This includes disallowing, for example, multiple LVM volume groups, multiple btrfs volumes, or combinations of the two. While these layouts are technically valid for a partitioned disk, we currently do not support them. - CheckDiskMountpointsPolicy(): Checks that none of the mountpoints are disallowed by the given PathPolicy. Tests added for all validators.
Define a nested map: [partition table type] -> [partition type name] -> [partition type ID] and use it from a private function getPartitionTypeIDfor() for convenient partition type ID selection.
New function that creates a partition table from scratch based on the new disk customization. First, it creates any necessary partitions for the selected boot mode (BIOS boot, ESP, or both). Then it determines if a /boot partition is needed based on the partitions and mountpoints defined in the disk customizations. Then, it iterates through partitions and creates each one in the order its defined. Finally, it creates a root filesystem if one wasn't already defined, resizes any necessary volumes, relayouts the table (setting all partition starts and offsets), and generates UUIDs all entities. Because of the new way the boot partition is handled, the EnsureBootPartition() function was changed to always add a boot partition and was renamed to AddBootPartition() to match the new behaviour. The tests have been updated accordingly.
Preparing the function to use the new PartitioningCustomization when set.
For all image types that don't specify a requiredPartitionSizes, add the defaults that are used in the NewPartitionTable() so that NewCustomPartitionTable() has similar behaviour. The only image type that explicitly doesn't specify minimum sizes is the iot-raw-image, which has a pre-existing empty map.
Add checks for Partitioning customizations in Fedora's checkOptions() function. The function now returns an error if: - Partitioning is defined at the same time as Filesystem - Partitioning is defined for ostree image types (commit and container). - The mountpoints in Partitioning violate the mountpoint policies for the image type.
c7b4411
to
f84f305
Compare
One for each variant: - Plain - Plain + btrfs - Plain + lvm
f84f305
to
6ba8dad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is good stuff, I have some ideas/comments/suggestions inline but no blockers (I think) and I think we can land this and do the nitpicks via a followup. I did not review (most of) the tests (a 4k review is hard) but I will later today/tomorow
func addPlainPartition(pt *PartitionTable, partition blueprint.PartitionCustomization, options *CustomPartitionTableOptions) error { | ||
fstype, err := options.getfstype(partition.FSType) | ||
if err != nil { | ||
return fmt.Errorf("error creating partition with mountpoint %q: %w", partition.Mountpoint, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) we could use this (in all add*) as an errPrefix := fmt.Sprintf("error creating partition with mountpoint %q:", partition.Mountpoint)
// for the substructure of a different type than the one defined in the "type" | ||
// fields. | ||
func (v *PartitionCustomization) UnmarshalJSON(data []byte) error { | ||
errPrefix := "JSON unmarshal:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of errPrefix if it is the same for all of the errors? I think it makes the code harder to read when jumping into the middle of it, and it also makes it impossible to grep for things like 'JSON unmarshal: foo failure at position bar'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An understandable concern (not being able to grep the source for a full error message). OTOH, it is a bit tedious to have the same string repeated throughout the function in every return and risking a typo or something like that changing just one of the error message prefixes. Idk which is better tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I did another pass over the test-cases, I think we miss a test for but we can easily add it in a followup, from my PoV this is good to go and all the comments/ideas can also be done/discussed in followups.
}`, | ||
expected: &blueprint.PartitionCustomization{ | ||
Type: "lvm", | ||
MinSize: 99 * datasizes.GiB, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(super nitpick) I wonder if we should just use MinSize: 106300440576
here, for the^Wmost readers it's not obvious that the former translated into 99GiB
input: `{"type":"not-a-partition-type"}`, | ||
errorMsg: "JSON unmarshal: unknown partition type: not-a-partition-type", | ||
}, | ||
"number": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(super nitpick) maybe "bad-type/number" ?
}, | ||
{ | ||
Start: 202 * datasizes.MiB, | ||
Size: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems slightly odd, so we create partitions of size 0
without minsize
constraints? I guess this will never happen in practice but is there a scenario where we would want this result or should we error if we do not get size constraints (RequiredMinSizes) for our auto-created partitions? (or am I just missing something here?)
pt.EnsureDirectorySizes(options.RequiredMinSizes) | ||
} | ||
|
||
pt.relayout(customizations.MinSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT we have no test for this value, yes? If I do:
diff --git a/pkg/disk/partition_table.go b/pkg/disk/partition_table.go
index 9bed8eb61..213414340 100644
--- a/pkg/disk/partition_table.go
+++ b/pkg/disk/partition_table.go
@@ -1237,7 +1237,7 @@ func NewCustomPartitionTable(customizations *blueprint.DiskCustomization, option
pt.EnsureDirectorySizes(options.RequiredMinSizes)
}
- pt.relayout(customizations.MinSize)
+ pt.relayout(0)
pt.GenerateUUIDs(rng)
return pt, nil
no test fails - should be easy enough to add though, the testing is so nice already which makes extending trivial
}, | ||
{ | ||
Start: 818 * datasizes.MiB, // the root vg is moved to the end of the partition table by relayout() | ||
Size: 3*datasizes.GiB + 16*datasizes.MiB + datasizes.MiB - (disk.DefaultSectorSize + (128 * 128)), // the sum of the LVs (rounded to the next 4 MiB extent) grows by 1 grain size (1 MiB) minus the unaligned size of the header to fit the gpt footer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) maybe instead of hm, maybe I should just let go but I struggle to see where the 4MiB extend comes from here128*128
just 4 * datasizes.MiB
?
expected: &disk.PartitionTable{ | ||
Type: disk.PT_GPT, // default when unspecified | ||
UUID: "0194fdc2-fa2f-4cc0-81d3-ff12045b73c8", | ||
Size: 818*datasizes.MiB + 16*datasizes.MiB + 3*datasizes.GiB + datasizes.MiB, // start + size of last partition (VG) + footer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) same as above, it becomes a bit tricky to follow where the 818 comes from, the comment seems to be a bit terse, so maybe a comment like // start + efi + boot + 2 VGs + rootsize
or something might slightly more helpful
expected: &disk.PartitionTable{ | ||
Type: disk.PT_GPT, // default when unspecified | ||
UUID: "0194fdc2-fa2f-4cc0-81d3-ff12045b73c8", | ||
Size: 714*datasizes.MiB + 168*datasizes.MiB + datasizes.MiB, // start + size of last partition (VG) + footer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) It becomes a bit tricky to follow where the 714 comes from, the comment seems to be a bit terse, so maybe a comment like // start + efi + boot + 3 LVs ` or something might slightly more helpful (same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A handful of docs/error reporting suggestions, but nothing major, thanks for this @achilleas-k , this is awesome!
Partitions []PartitionCustomization `json:"partitions,omitempty" toml:"partitions,omitempty"` | ||
} | ||
|
||
// PartitionCustomization defines a single partition on a disk. The Type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the type descriptions should go to the Type
field?
// The type of payload for the partition (optional, defaults to "plain"). | ||
Type string `json:"type" toml:"type"` | ||
|
||
// Minimum size of the partition that contains the filesystem (for "plain" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the default MinSize for a plain partition? Should it actually be required for plain?
if lvAnySize.MinSize != nil { | ||
size, err := decodeSize(lvAnySize.MinSize) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapping this would be nice
This commit changes some ordering and var names, very minor, test tweaks, c.f. * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment)
This commit changes some ordering and var names, very minor, test tweaks, c.f. * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment)
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
This commit changes some ordering and var names, very minor, test tweaks, c.f. * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment)
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Follows, c.f. osbuild#1041 (comment) osbuild#1041 (comment)
Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Followups, c.f. osbuild#1041 (comment) osbuild#1041 (comment)
This commit extracts a new `maybeAddBootPartition()` helper to make the `NewCustomPartitionTable()` a little bit more linear to read. Followup, c.f. osbuild#1041 (comment)
Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Followups, c.f. osbuild#1041 (comment)
This commit extracts a new `maybeAddBootPartition()` helper to make the `NewCustomPartitionTable()` a little bit more linear to read. Followup, c.f. osbuild#1041 (comment)
Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Followups, c.f. #1041 (comment)
This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. osbuild#1041 (comment)
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
This commit extracts a new `maybeAddBootPartition()` helper to make the `NewCustomPartitionTable()` a little bit more linear to read. Followup, c.f. #1041 (comment)
This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. osbuild#1041 (comment)
This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. #1041 (comment)
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
Small non-functional tweaks for the disk customization tests. C.f.: osbuild#1041 (comment) osbuild#1041 (comment)
This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.
This is the main part of #926 which I split into multiple smaller PRs. With this PR, all of the functionality from the original one is implemented, with some changes to the customization format.
This PR adds a new advanced partitioning customization to the blueprint that has the following structure:
When the new customizations are used, the base partition table for the image type is ignored. Instead, a new "custom" partition table is constructed from scratch by iterating through the customizations and creating partitions, volume groups and logical volumes, and btrfs volumes and subvolumes as needed. Boot, EFI, and root partitions are created automatically to make the partition table valid and usable. Parameters such as boot mode and partition table type should be determined by the caller of the function, which should be based on the distro, architecture, and image type. This is, in a way, much simpler than our existing
ensureLVM()
andensureBtrfs()
functions, which needed to modify an existing partition table's structure in a way that preserved some parts and not others.The custom partition table creation function (
NewCustomPartitionTable()
) supports all kinds of valid but perhaps unwanted configurations. For example, it's possible to create a partition table with both LVM volume groups and btrfs volumes, or multiples of each. The customizations however don't allow this, but the structure supports it (see above) in case we want to expand what we support in the future.Customization validation happens in the blueprint. Invalid values such as duplicate mountpoints, empty btrfs subvolume names, etc, are caught by a validator in the blueprint code itself.
What's next:
DRAFT